Support reentrant calls to RakeCommand.perform #39137
Conversation
| Rake.application.load_rakefile | ||
| Rake.application.top_level | ||
| Rake.with_application do |rake| | ||
| load "rails/tasks.rb" |
There was a problem hiding this comment.
It's unfortunate that this file has to be loaded each time, but I could not find a sanctioned way to copy loaded tasks from one instance to another. Neither Rake::Application nor Rake::TaskManager seem to have the necessary public accessors.
There was a problem hiding this comment.
Will reloading throw any constant redefined or similar warnings? Is it tough on performance?
Not sure if we know someone involved with Rake we could tap to help fit an API for this.
There was a problem hiding this comment.
There was one constant redefined warning that I fixed: https://github.com/rails/rails/pull/39137/files#diff-94b3f444d3f09e55370aa7e94f440408R6
As for performance, I'm sure it is slower, but I didn't really notice the difference when I was repeatedly running tasks to try out the fix. And it should still beat shelling out and loading the entire Rails context in another process, which is what was happening before adding inline: true.
It would be cool if Rake supported this use case explicitly. Depending on how common this use case is, an option like Rake.with_application(preserve_tasks: true) would be pretty slick. Otherwise, something like Rake::Application#dup_without_top_level would be next best thing.
jonathanhefner
force-pushed
the
reentrant-rake_command-perform
branch
from
ecbfab0
to
6971f3b
Compare
Rake stores the current top-level task and arguments in global state. Invoking another top-level task within the same process requires overwriting this state. Doing so indiscriminately can cause incorrect behavior, such as infinitely repeating the original task. In particular, this is a problem when running one task from another via `rails_command "...", inline: true`. The solution is to save and restore the global state in each call to `RakeCommand.perform` using the `Rake.with_application` method. Fixes rails#39128.
jonathanhefner
force-pushed
the
reentrant-rake_command-perform
branch
from
6971f3b
to
68a3f67
Compare
|
Thanks! |
|
@jonathanhefner Do you have any thoughts on this issue I'm encountering where commands are run twice? Reverting your change appears to stop that particular issue: #40136 |
In rails#39137, a new `Rake::Application` instance was created per Rake command invocation. To ensure that the Rails tasks were defined for each `Rake::Application`, `rails/tasks.rb` was loaded per instance. However, `Rake::Application#load_rakefile` loads the application's Rakefile, which should invoke `Rails.application.load_tasks`, which, in turn, also loads the Rails tasks. When a Rake task is defined more than once, all definition blocks are executed when the task is run. Hence, Rails task blocks were being executed twice. This commit removes the unnecessary load and avoids double execution. Fixes rails#40136.
Rake stores the current top-level task and arguments in global state. Invoking another top-level task within the same process requires overwriting this state. Doing so indiscriminately can cause incorrect behavior, such as infinitely repeating the original task. In particular, this is a problem when running one task from another via
rails_command "...", inline: true.The solution is to save and restore the global state in each call to
RakeCommand.performusing theRake.with_applicationmethod.Fixes #39128.